perf: array_agg() performance improvements - #23716
Open
fred1268 wants to merge 8 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23716 +/- ##
==========================================
- Coverage 80.69% 80.69% -0.01%
==========================================
Files 1089 1089
Lines 368525 368602 +77
Branches 368525 368602 +77
==========================================
+ Hits 297392 297447 +55
- Misses 53414 53426 +12
- Partials 17719 17729 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
run benchmarks array_agg |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/array_aggregate (33c5bb1) to 1e58928 (merge-base) diff using: array_agg File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagearray_agg — base (merge-base)
array_agg — branch
File an issue against this benchmark runner |
Contributor
|
Seems like there's no impact in those benchmarks. Do you know if there are any other benchmarks that are using |
Contributor
|
Let's ship the benchmarks in a preliminary PR, and then come back to this one. |
fred1268
force-pushed
the
perf/array_aggregate
branch
from
July 21, 2026 09:02
33c5bb1 to
1cd1305
Compare
Contributor
|
run benchmarks array_agg |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/array_aggregate (1cd1305) to 1fcdef2 (merge-base) diff using: array_agg File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagearray_agg — base (merge-base)
array_agg — branch
File an issue against this benchmark runner |
Contributor
|
Nice! |
gabotechs
changed the title
array_agg() performance improvementsarray_agg() performance improvements
Contributor
Author
|
@jayzhan211 @alamb as the original author and reviewer of this code, you may be interested this PR. |
alamb
previously approved these changes
Jul 24, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thanks @fred1268 -- looks good to me amd is a nice improvement -- Rows will likely be much faster than ScalarValue as borne by the benchmarks
I (along with claude code) found two issues with this PR that were not picked up in regression tests:
- It breaks
array_aggon dictionaries - It has unbounded memory growth for retract_rows (aka when used as a window function)
I made a PR that adds test coverage here:
I also had a few style suggestions, but nothing critical.
I do think we need to fix the regressions though
| assert_eq!(acc1.size(), 1684); | ||
| // The GroupValuesRows-based implementation uses a contiguous Rows | ||
| // buffer + HashTable instead of individual ScalarValue allocations, | ||
| // so the reported size differs from the previous implementation (was 1684). |
Contributor
There was a problem hiding this comment.
I think the context about the previous impleementation will not be relevant after this PR merges -- it is more a comment for this PR -- just updating without this comment I think would be less confusing
Contributor
Author
There was a problem hiding this comment.
Sorry about this, I forgot to clean them up
| // reads keys. Refcount semantics for retract are only valid within | ||
| // a single accumulator instance (window execution). | ||
| // The DISTINCT state is `List<value>`. Partial accumulators ship the | ||
| // set of live values, not multiplicities. Re-ingesting them here is |
Contributor
There was a problem hiding this comment.
what are multiplicities? I think DISTINCT state is List<value> is probably enough for this comment (i see you dodn't add this)
Contributor
Author
There was a problem hiding this comment.
Sorry about this, I forgot to clean them up
| }); | ||
| delayed_cmp_err?; | ||
| }; | ||
| let group_values = self |
Contributor
There was a problem hiding this comment.
You can likely avoid these expects / implicit state machines by using an explicit state enum . Maybe something like this:
enum state {
Init,
Allocated {
map: ...,
group_values: ...:
converter: ...
}
}
Contributor
Author
There was a problem hiding this comment.
I implemented something that is my understanding of your comment.
Please let me know if it is not the case.
alamb
dismissed
their stale review
July 24, 2026 21:20
Clicked wrong button -- need to fix regressions first
athlcode
pushed a commit
to athlcode/datafusion
that referenced
this pull request
Jul 25, 2026
…t_batch` memory (apache#23873) ## Which issue does this PR close? - related to apache#23716 ## Rationale for this change It adds test coverage for two gaps found while reviewing apache#23716. ## What changes are included in this PR? Tests only, no functional change. - Dictionary inputs - memory usage on retract (make sure memory is released) Note I moved `array_agg` cases out `aggregate.slt` as it is already more than 9k lines long ## Are these changes tested? They are only tests ## Are there any user-facing changes? No. Tests only, no public API changes. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
alamb
reviewed
Jul 28, 2026
|
|
||
| /// Resources that are allocated lazily on the first `update_batch` call, | ||
| /// once the concrete runtime Arrow type is known. | ||
| /// |
Contributor
There was a problem hiding this comment.
What would be amazing is if we could reuse some of the actual hash aggregate implementations -- but I don't really know how to do that yet
Contributor
|
Thanks @fred1268 -- I merged up from main to resolve a conflict. The code looks good to me now, though it may be slower as it now does a new allocation for each distinct value (OwnedRow). I'll do the benchmarks one more time |
Contributor
|
run benchmarks array_agg |
alamb
approved these changes
Jul 28, 2026
| /// `.expect()` calls that would otherwise be needed. | ||
| #[derive(Debug)] | ||
| pub struct DistinctArrayAggAccumulator { | ||
| struct DistinctState { |
| group_values: Rows, | ||
| /// One owned encoded row per live distinct value, indexed by group index. | ||
| /// Compacted via swap-remove on eviction so there are never dead slots. | ||
| group_rows: Vec<OwnedRow>, |
Contributor
There was a problem hiding this comment.
If this turns out to be too expenive (an allocation per OwnedRow) we could also use another Rows here, though managing it will be complicated as Rows doens't have a swap_remove function
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/array_aggregate (8389f2e) to bb75d92 (merge-base) diff using: array_agg File an issue against this benchmark runner |
Contributor
Author
|
Thanks @alamb for the thorough review, and sorry if I pushed in two batches which was confusing for you (will try to do better next time). Let me know if there is something else I can do on this PR. |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagearray_agg — base (merge-base)
array_agg — branch
File an issue against this benchmark runner |
Contributor
Author
|
The high cardinality benchmark, although still an improvement, is not as good as in the previous version. That being said, I would expect most people will do |
Contributor
|
Performance results definitely look like an improvement to me |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
array_agg()performance #23715.Rationale for this change
Queries using
array_agg(DISTINCT col)were significantly slower than expected.Profiling revealed that
DistinctArrayAggAccumulator::update_batchwas allocating aheap-owned
Stringon every single input row — even for rows whose value was alreadypresent in the accumulator. For a typical low-cardinality workload (e.g. a column of ~25
database names across thousands of rows), this meant paying the full allocation cost for
every duplicate, which dominated the runtime.
What changes are included in this PR?
This PR applies the same deduplication strategy already used by
AggregateExecforGROUP BY: duplicate rows now cost only a hash probe with no heap allocation, and newdistinct values are appended to a single shared buffer rather than allocated individually.
The fix applies to all column types, not just strings, and
retract_batchsupport(required for sliding window frames such as
ROWS BETWEEN N PRECEDING AND CURRENT ROW)is fully preserved.
Are these changes tested?
Four unit tests were added to
DistinctArrayAggAccumulator— one each forUtf8,Int64,Float64, andDate32— to pin the deduplication contract across the mostcommon column types and serve as a regression guard for future changes. The existing
sliding window sqllogictest suite (
array_agg_sliding_window.slt) coversretract_batchcorrectness end-to-end and passes unchanged.
Two
update_batchmicro-benchmarks were added to measure the before/after on realisticdata: one with low cardinality (~25 distinct database names in 8 192 rows, modelling the
common production case) and one with high cardinality (~7 800 distinct values, modelling
the worst case where almost every row is new). Results on an 8 192-row batch:
Are there any user-facing changes?
No user-facing changes
No breaking changes to public APIs